Skip to content

Fix GitHub Actions: Prevent Command Injection in Firefox Workflow#989

Open
PeterDaveHello wants to merge 1 commit into
masterfrom
codex/fix-command-injection-vulnerability-in-workflow
Open

Fix GitHub Actions: Prevent Command Injection in Firefox Workflow#989
PeterDaveHello wants to merge 1 commit into
masterfrom
codex/fix-command-injection-vulnerability-in-workflow

Conversation

@PeterDaveHello

@PeterDaveHello PeterDaveHello commented Jun 30, 2026

Copy link
Copy Markdown
Member

Motivation

  • Detected that .github/workflows/firefox-metadata.yml directly expands workflow_dispatch.inputs.version into a shell run step, which can lead to command injection and potentially expose FIREFOX_* secrets.
  • The goal is to eliminate the injection risk with minimal changes while preserving the existing Firefox metadata update workflow.

Description

  • Changed the workflow to pass the user input through the FIREFOX_METADATA_VERSION environment variable and reference it in the shell as "$FIREFOX_METADATA_VERSION" to prevent GitHub Actions expressions from being expanded before shell parsing.
  • Added AMO-compatible numeric version validation for x.y.z and x.y.z.w, allowing up to nine digits per component while rejecting leading zeros. The step aborts before running npm run release:update-firefox-metadata if the input does not match.
  • Explicitly selected Bash for the step that uses [[ ... =~ ... ]] validation.
  • Modified file: .github/workflows/firefox-metadata.yml.

Testing

  • Ran npm test; all automated unit tests passed with no failures.
  • Ran npm run lint; static analysis passed with no lint errors.
  • Ran npm run build and verified that the expected artifacts exist in build/chromium/, and that git diff --check reported no issues.

Codex Task

Summary by CodeRabbit

Summary by CodeRabbit

  • Bug Fixes
    • Improved Firefox metadata updates by validating the provided Firefox version format before running the update (supports both x.y.z and x.y.z.w).
    • The workflow now fails fast with a clear error message when the version is malformed, reducing chances of incorrect release metadata.
    • Added an example to the manual version input to clarify the accepted 4-part version format.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Note

Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported.

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The Firefox metadata workflow now documents three- or four-component version formats and validates FIREFOX_METADATA_VERSION before running the update command. Invalid values cause the step to exit with an error, and the version is supplied through the step environment.

Changes

Workflow Version Validation

Layer / File(s) Summary
Document and validate Firefox version input
.github/workflows/firefox-metadata.yml
The workflow adds a four-component version example and validates FIREFOX_METADATA_VERSION against three- or four-component numeric formats without leading zeros before invoking the metadata update command.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Suggested reviewers: josstorer

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: hardening the Firefox GitHub Actions workflow against command injection.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-command-injection-vulnerability-in-workflow

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Harden Firefox metadata GitHub Action against command injection

🐞 Bug fix ⚙️ Configuration changes 🕐 Less than 10 minutes

Grey Divider

AI Description

• Pass workflow input via env var and quote it in bash to prevent shell injection.
• Add strict x.y.z semver validation and fail fast on invalid versions.
• Keep existing npm-based Firefox metadata update flow unchanged otherwise.
Diagram

graph TD
  A["workflow_dispatch input: version"] --> B["Env: FIREFOX_METADATA_VERSION"] --> C{"Semver x.y.z?"} -->|"yes"| D["Run npm update script"] --> E["Update Firefox metadata"]
  C -->|"no"| F["Exit 1 + error"]
  G[("FIREFOX_* secrets")] --> D

  subgraph Legend
    direction LR
    _in["Input"] ~~~ _dec{"Decision"} ~~~ _proc["Process"] ~~~ _sec[("Secrets")]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Validate with a semver tool (node/semver or npx semver)
  • ➕ More accurate semver handling (pre-releases/build metadata if desired)
  • ➕ Clearer error messages and maintainability vs bash regex
  • ➖ Adds dependency/tooling surface area to a workflow step
  • ➖ Slightly more moving parts for a minimal security fix
2. Use workflow_dispatch input constraints (choices) when feasible
  • ➕ Prevents invalid values before the runner starts
  • ➕ No shell parsing involved for validation
  • ➖ Not practical for arbitrary version values (needs enumerated list)
  • ➖ Still should quote/handle safely in the step
3. Add hardened shell defaults (set -euo pipefail) and explicit shell
  • ➕ Improves safety and debuggability across the step
  • ➕ Reduces risk from unset variables or silent failures
  • ➖ Can be a behavioral change if the script currently relies on unset vars
  • ➖ Still requires proper quoting/validation of inputs

Recommendation: The PR’s approach (env indirection + strict quoting + allowlist validation) is the best minimal-change mitigation for command injection in a workflow run step. Consider optionally adding set -euo pipefail for additional robustness, but the core security issue is already addressed effectively.

Files changed (1) +8 / -1

Bug fix (1) +8 / -1
firefox-metadata.ymlAvoid shell injection from dispatch input and validate version +8/-1

Avoid shell injection from dispatch input and validate version

• Replaces direct interpolation of 'inputs.version' in the 'run' command with an environment variable ('FIREFOX_METADATA_VERSION') used in a quoted context. Adds a strict x.y.z semver regex check to fail fast on invalid input before running the npm metadata update script.

.github/workflows/firefox-metadata.yml

@PeterDaveHello PeterDaveHello changed the title 修補 GitHub Actions:防止 Firefox metadata workflow 指令注入 Fix GitHub Actions: Prevent Command Injection in Firefox Workflow Jun 30, 2026
@PeterDaveHello
PeterDaveHello requested a review from Copilot June 30, 2026 19:34
@qodo-code-review

qodo-code-review Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Informational

1. Implicit bash dependency ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
此步驟新增了 bash 專用的 [[ ... =~ ... ]] 正則判斷,但 workflow/step 未明確指定 shell: bash,未來若 workflow 預設 shell
被調整(或重用到非 bash 環境)會導致語法錯誤而使更新步驟失敗。
Code

.github/workflows/firefox-metadata.yml[R30-34]

+        run: |
+          if [[ ! "$FIREFOX_METADATA_VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
+            echo "Firefox version must use x.y.z semver format" >&2
+            exit 1
+          fi
Evidence
在本 PR 新增的 run 腳本中使用了 [[ ... =~ ... ]];同一個 workflow 檔案中並未看到 step-level shell:defaults.run.shell 設定,因此這段腳本對預設 shell 行為形成隱含依賴。

.github/workflows/firefox-metadata.yml[14-41]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The workflow step relies on bash-specific syntax (`[[ ... =~ ... ]]`) but does not explicitly pin the shell. This creates a portability/maintenance hazard: if the workflow’s default shell changes or the job is later adapted to a different runner/shell, the step may fail with a syntax error.

### Issue Context
The current step uses `[[` and `=~` for regex validation. These are not POSIX `sh` features.

### Fix Focus Areas
- .github/workflows/firefox-metadata.yml[30-36]

### Suggested fix
Add `shell: bash` to the step (or define `defaults: run: shell: bash` for the job/workflow), **or** rewrite the validation in POSIX-compatible `sh` (e.g., using `case`/`grep -E`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit dd8d650

Results up to commit 79436f1


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Informational
1. Implicit bash dependency ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
此步驟新增了 bash 專用的 [[ ... =~ ... ]] 正則判斷,但 workflow/step 未明確指定 shell: bash,未來若 workflow 預設 shell
被調整(或重用到非 bash 環境)會導致語法錯誤而使更新步驟失敗。
Code

.github/workflows/firefox-metadata.yml[R30-34]

+        run: |
+          if [[ ! "$FIREFOX_METADATA_VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
+            echo "Firefox version must use x.y.z semver format" >&2
+            exit 1
+          fi
Evidence
在本 PR 新增的 run 腳本中使用了 [[ ... =~ ... ]];同一個 workflow 檔案中並未看到 step-level shell:defaults.run.shell 設定,因此這段腳本對預設 shell 行為形成隱含依賴。

.github/workflows/firefox-metadata.yml[14-41]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The workflow step relies on bash-specific syntax (`[[ ... =~ ... ]]`) but does not explicitly pin the shell. This creates a portability/maintenance hazard: if the workflow’s default shell changes or the job is later adapted to a different runner/shell, the step may fail with a syntax error.

### Issue Context
The current step uses `[[` and `=~` for regex validation. These are not POSIX `sh` features.

### Fix Focus Areas
- .github/workflows/firefox-metadata.yml[30-36]

### Suggested fix
Add `shell: bash` to the step (or define `defaults: run: shell: bash` for the job/workflow), **or** rewrite the validation in POSIX-compatible `sh` (e.g., using `case`/`grep -E`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

Comment thread .github/workflows/firefox-metadata.yml

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request hardens the Firefox metadata GitHub Actions workflow against command injection by ensuring the user-provided workflow_dispatch version input is treated as data (not shell code) and is validated before use.

Changes:

  • Routes the inputs.version value through an environment variable and references it as "$FIREFOX_METADATA_VERSION" in the shell step.
  • Adds strict x.y.z semantic version validation (^[0-9]+\.[0-9]+\.[0-9]+$) and fails fast on invalid input.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@PeterDaveHello
PeterDaveHello requested a review from josStorer July 4, 2026 19:18
@kilo-code-bot

kilo-code-bot Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (1 file)
  • .github/workflows/firefox-metadata.yml

Incremental review note: the prior inline concern about the implicit bash dependency (use of [[ ... =~ ... ]]) is now resolved by the addition of shell: bash on the step. The version input is now passed via the FIREFOX_METADATA_VERSION environment variable and validated against a strict numeric format before use, eliminating the command-injection risk.

Previous Review Summaries (2 snapshots, latest commit 2f820a0)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 2f820a0)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (1 file)
  • .github/workflows/firefox-metadata.yml

Previous review (commit 79436f1)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (1 file)
  • .github/workflows/firefox-metadata.yml

Reviewed by hy3:free · Input: 22.5K · Output: 1.9K · Cached: 136.2K

@PeterDaveHello
PeterDaveHello force-pushed the codex/fix-command-injection-vulnerability-in-workflow branch from 79436f1 to 2f820a0 Compare July 5, 2026 08:04
@PeterDaveHello

Copy link
Copy Markdown
Member Author

@josStorer would you like to take a look at this PR?

Match Firefox numeric manifest versions, including historical
four-segment versions, while rejecting leading-zero components.
@PeterDaveHello
PeterDaveHello force-pushed the codex/fix-command-injection-vulnerability-in-workflow branch from 2f820a0 to dd8d650 Compare July 16, 2026 19:03
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Note

Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported.

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit dd8d650

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
.github/workflows/firefox-metadata.yml (1)

18-19: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Disable credential persistence to improve security posture.

If npm run release:update-firefox-metadata does not push commits back to the repository, you can safely disable credential persistence in the checkout action. This adheres to the principle of least privilege by preventing subsequent steps or dependencies from unnecessarily accessing the GitHub token in the runner environment.

🛡️ Proposed fix to disable credential persistence
       - name: Checkout
         uses: actions/checkout@v7
+        with:
+          persist-credentials: false
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/firefox-metadata.yml around lines 18 - 19, Update the
Checkout step using actions/checkout@v7 to disable credential persistence by
setting persist-credentials to false, preserving the existing checkout behavior.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In @.github/workflows/firefox-metadata.yml:
- Around line 18-19: Update the Checkout step using actions/checkout@v7 to
disable credential persistence by setting persist-credentials to false,
preserving the existing checkout behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7eb36851-454e-41d3-82f3-a745e6c0d4c0

📥 Commits

Reviewing files that changed from the base of the PR and between 2f820a0 and dd8d650.

📒 Files selected for processing (1)
  • .github/workflows/firefox-metadata.yml

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread .github/workflows/firefox-metadata.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants